Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @reggev)

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 49 at r1 (raw file):

JSON.parse(storage)

JSON.parse throws an exception if it cannot parse

core/datasources-service/api/rest-api/swagger/definitions/create.request.yaml, line 21 at r1 (raw file):

        type: object
        properties:
            organization:

just org? no need for user?

core/datasources-service/config/main/config.base.js, line 85 at r1 (raw file):

GIT_ENDPOINT_URL

If you require the protocol, we need to change the helm. Just a reminder

core/datasources-service/config/main/config.base.js, line 90 at r1 (raw file):

    baseDirectory:
        process.env.BASE_FS_ADAPTER_DIRECTORY || '/var/tmp/fs/storage',
    baseDatasourcesDirectory:

why such short lines? In my editor window I can easily fit 120-150 chars. No need to fold the lines at 80

core/datasources-service/lib/service/downloads.js, line 73 at r1 (raw file):

`${this.config.directories.prepareForDownload}/${downloadId}`

path.join()

core/datasources-service/lib/service/jobs-consumer.js, line 122 at r1 (raw file):

            dataSource.name,
            this.config,
            `${this.rootDir}/${dataSource.name}/${dataSource.id}`

path.join()

core/datasources-service/lib/utils/dvcConfig.js, line 7 at r1 (raw file):

/** @param {ExternalStorage} config */
const S3Config = ({
    endpoint,

for user provided S3, we also need bucket name

core/datasources-service/lib/utils/Repository.js, line 3 at r1 (raw file):

const { Octokit: GitRestClient } = require('@octokit/rest');

you need an abstraction over the git client, so you can support both github and gitlab

core/datasources-service/lib/utils/Repository.js, line 37 at r1 (raw file):

repositoryName

Why do you need to pass dir-name separately? Is there a use case where it is not the repo name?

core/datasources-service/lib/utils/Repository.js, line 63 at r1 (raw file):

        } else {
            const url = new URL(value);
            url.username = this.gitConfig.token;

You have an implicit precondition, that the token has been set before calling the setter. This is usually unexpected in property setter.
Better create a method that accepts both token and url

Reviewable status: 8 of 27 files reviewed, 10 unresolved discussions (waiting on @yehiyam)

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 49 at r1 (raw file):

Done.

core/datasources-service/api/rest-api/swagger/definitions/create.request.yaml, line 21 at r1 (raw file):

there's a token property it contains all you need (at least for github)

core/datasources-service/config/main/config.base.js, line 85 at r1 (raw file):

At this point this is used for testing only, the user sets this endpoint per dataSource on creation

core/datasources-service/config/main/config.base.js, line 90 at r1 (raw file):

It's not about space, it's because wide lines are just not readable...

core/datasources-service/lib/service/downloads.js, line 73 at r1 (raw file):

Done.

core/datasources-service/lib/service/jobs-consumer.js, line 122 at r1 (raw file):

Done.

core/datasources-service/lib/utils/dvcConfig.js, line 7 at r1 (raw file):

Done.

core/datasources-service/lib/utils/Repository.js, line 3 at r1 (raw file):

Done.

core/datasources-service/lib/utils/Repository.js, line 37 at r1 (raw file):

The cli can have that gap between the repository name and the directory
this is why I added the hkube file to not depend on the directory's name

core/datasources-service/lib/utils/Repository.js, line 63 at r1 (raw file):

Done.

Reviewed 21 of 21 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @reggev)

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 49 at r2 (raw file):

            let storageConfig;
            try {
                gitConfig = git ? JSON.parse(git) : undefined;

Is this json in json?

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 53 at r2 (raw file):

            } catch (error) {
                throw new InvalidDataError(
                    "you provided invalid 'git' or 'storage' settings"

Don't use "second person (you)"

core/datasources-service/config/main/config.base.js, line 90 at r1 (raw file):

80 lines are not readable as well... everything is split into 2-3 lines

core/datasources-service/lib/utils/GitRemoteClient/Github.js, line 47 at r2 (raw file):

    async deleteRepository() {
        throw new Error('Github deleteRepository: not implemented!');

Is there a reason it is not implemented? or not implemented yet?

Reviewable status: 24 of 31 files reviewed, 4 unresolved discussions (waiting on @yehiyam)

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 49 at r2 (raw file):

yes it is a stringified JSON data sent as form data

core/datasources-service/api/rest-api/routes/v1/datasource.js, line 53 at r2 (raw file):

I did a search and eliminated all the instances of "you" in the repository

core/datasources-service/config/main/config.base.js, line 90 at r1 (raw file):

80 is the common practice and it is much much better than going wide

core/datasources-service/lib/utils/GitRemoteClient/Github.js, line 47 at r2 (raw file):

it is implemented in another branch and will be merged later on

Reviewed 7 of 7 files at r3, 5 of 5 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved